-
-
Notifications
You must be signed in to change notification settings - Fork 620
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Check that precision of extension_api.json
matches build options
#1714
base: master
Are you sure you want to change the base?
Conversation
@enetheru I'll eventually need your help figuring out the best way to do this in CMake. Maybe actually this check should be moved into |
@dsnopek CMake gained the ability to parse json in 3.19, we are on 3.17 https://cmake.org/cmake/help/latest/command/string.html#json Looks simple enough to use, if we are ok with the cmake version bump. |
93b484e
to
0a73df5
Compare
We're already using Python, and have the data available in Python, so I just moved the check into |
How do I go about testing this? |
That would work, although, an easier way to test is just to add |
OK, that was much easier, I can confirm that a build will fail using cmake with an error message that is legible enough to understand the problem, even though it looks a bit garbled. Would be nicer to fail during the configure phase rather than the build phase in case of larger projects that compile a bunch of stuff prior to godot-cpp. I'll see if i can whip something up with cmake 3.19 quickly being able to parse json directly with cmake might come in handy in the future. Our CI's are already updated to Ubuntu 22.0.4 which has a CMake version of 3.22.1 MinGW64 + Ninja
msvc
|
If the API contains the precision and its invalid to compile with a differing result, then perhaps source the precision directly from the API and remove it as an option from the compile commands? Anyway, this is a quick draft of something that fails at configure time with context at the top, in godot-cpp.cmake at roughly line 223 # API json File
set(GODOT_GDEXTENSION_API_FILE "${GODOT_GDEXTENSION_DIR}/extension_api.json")
if( GODOT_CUSTOM_API_FILE ) # User-defined override.
set(GODOT_GDEXTENSION_API_FILE "${GODOT_CUSTOM_API_FILE}")
endif()
# Check for correct precision
file(READ ${GODOT_GDEXTENSION_API_FILE} API_JSON )
string(JSON API_PRECISION ERROR_VARIABLE API_ERROR GET "${API_JSON}" "header" "precision")
if( API_ERROR )
message(WARNING "In file '${GODOT_GDEXTENSION_API_FILE}': \"precision\" is missing from the header object."
" It must have been created prior to 4.4, continuing with the assumption you are using the correct api file."
" See https://github.com/godotengine/godot-cpp/pull/1714 for more information.")
elseif( NOT API_PRECISION STREQUAL GODOT_PRECISION )
message( FATAL_ERROR "Cannot do a precision=${GODOT_PRECISION} build using '${GODOT_GDEXTENSION_API_FILE}'"
" which was generated by Godot built with precision=${API_PRECISION}" )
endif() Edited to make it a little nicer. |
@enetheru Thanks for testing and investigation!
That's about on par with the current message from SCons. It was also somewhat more legible in my first SCons implementation that used But I personally think this is fine for now, if it gets us better compatibility - we can always improve it later. I'd be curious to hear what folks with more SCons knowledge think of this, though.
I'm not crazy about this idea, because it doesn't explain to the user why the option isn't available. Although, I suppose it could be paired with a warning message when the logic is deciding to remove the option?
Thanks! What does the error message look like with that implementation? |
With older api.json without the header tag and lets you continue with a warning:
With a json file with mismatching header.precision that fails at configure time:
|
Godot PR merged, this is now ready for review! |
extension_api.json
matches build optionsextension_api.json
matches build options
Depends on godotengine/godot#103137
We regularly get bug reports from folks who compile their GDExtension with
precision=double
, but use anextension_api.json
from a single precision build of Godot.This PR uses an additonal field that godotengine/godot#103137 adds to the
extension_api.json
to validate that the file matches the build optionsMarking as DRAFT until the Godot PR is merged
(It should be fine to merge this after the Godot PR is merged, but even before it's in a stable release of Godot, because it won't do anything if the new field is missing from the
extension_api.json
)